Skip to content

feat: add 1st batch of filters#20

Merged
mariajgrimaldi merged 1 commit intomainfrom
MJG/1st_filters_batch
Dec 15, 2021
Merged

feat: add 1st batch of filters#20
mariajgrimaldi merged 1 commit intomainfrom
MJG/1st_filters_batch

Conversation

@mariajgrimaldi
Copy link
Member

@mariajgrimaldi mariajgrimaldi commented Nov 25, 2021 •
edited
Loading

Description:

This PR adds:

  • PreRegisterFilter: intended to be executed before the registration process starts.
  • PreLoginFilter: intended to be executed before the login process starts.

Used by edx-platform.

Testing instructions:

  1. Install openedx-filters using this branch.
  2. Use this edx-platform branch MJG/1st_filters_batch
  3. In your favorite plugin, create some filter steps. For example:
class ModifyUsernameBeforeRegistration(PipelineStep):

    def run_filter(self, form_data):
        username = f"{form_data.get('username')}-edunext"
        form_data["username"] = username
        return {
            "form_data": form_data,
        }

These sample filter steps are implemented in this plugin

  1. Add the following Django settings to your configuration file:
OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.course.enrollment.started.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyModeBeforeEnrollment",
            "openedx_filters_samples.samples.pipeline.StopEnrollment"
        ]
    },
    "org.openedx.learning.student.login.requested.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyUserProfileBeforeLogin",
            "openedx_filters_samples.samples.pipeline.StopLogin"
        ]
    },
    "org.openedx.learning.student.registration.requested.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyUsernameBeforeRegistration",
            "openedx_filters_samples.samples.pipeline.StopRegister"
        ]
    }
}

With this configuration, you won't be able to register, login, or enroll in a course. Another configuration you could try is, instead of Stop<process> use NoopFilter

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

mariajgrimaldi
mariajgrimaldi commented Nov 27, 2021
View reviewed changes
openedx_filters/learning/auth.py Outdated
@@ -0,0 +1,54 @@
"""
Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Nov 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move all filters to learning/filters.py file? @felipemontoya to be compliant with openedx-events

Copy link
Member

@felipemontoya felipemontoya Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have indeed most signal definitions for events at learning/signals.py, which suggests that we should also move to learning/filters.py.

If we don't, then we will have a filter:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.auth.PreRegisterFilter.

If we do, then we are looking at:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.filters.PreRegisterFilter.

which is a little more consistent with from openedx_events.learning.signals import SESSION_LOGIN_COMPLETED which is a live example from the events implementation.

felipemontoya
felipemontoya requested changes Dec 14, 2021
View reviewed changes
openedx_filters/learning/auth.py Outdated
@@ -0,0 +1,54 @@
"""
Copy link
Member

@felipemontoya felipemontoya Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have indeed most signal definitions for events at learning/signals.py, which suggests that we should also move to learning/filters.py.

If we don't, then we will have a filter:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.auth.PreRegisterFilter.

If we do, then we are looking at:
"org.openedx.learning.student.registration.requested.v1"
defined at openedx_filters.learning.filters.PreRegisterFilter.

which is a little more consistent with from openedx_events.learning.signals import SESSION_LOGIN_COMPLETED which is a live example from the events implementation.

openedx_filters/learning/auth.py Outdated
from openedx_filters.tooling import OpenEdxPublicFilter


class PreRegisterFilter(OpenEdxPublicFilter):
Copy link
Member

@felipemontoya felipemontoya Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I find the names "PreRegisterFilter" or "PreLoginFilter" very easy to understand, I think that we set an important precedent at the events library. There we have:

COURSE_ENROLLMENT_CREATED:
    event_type="org.openedx.learning.course.enrollment.created.v1"

STUDENT_REGISTRATION_COMPLETED:
    event_type="org.openedx.learning.student.registration.completed.v1"

COURSE_UNENROLLMENT_COMPLETED
    event_type="org.openedx.learning.course.unenrollment.completed.v1"

While in this PR we are proposing:

PreRegisterFilter:
    filter_type = "org.openedx.learning.student.registration.requested.v1"

We could change this so that the name matches a little better the filter_type.
For example we could use any of the following as the class names:

  • StudentRegistrationRequested
  • StudentRegistrationStarted
  • StudentRegistrationFilter

Copy link
Member

@felipemontoya felipemontoya Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably also affects the naming of the PreEnrollmentFilter and the implementing PR as well

Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion. I'll go with the StudentRegistrationRequested because it's the closest to the events naming convention.

openedx_filters/learning/auth.py Outdated

filter_type = "org.openedx.learning.student.registration.requested.v1"
sensitive_form_data = [
"password",
Copy link
Member

@felipemontoya felipemontoya Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is password enough? should we be censoring all the fields at https://github.com/edx/edx-platform/blob/master/common/djangoapps/track/middleware.py#L64

felipemontoya
felipemontoya reviewed Dec 14, 2021
View reviewed changes
felipemontoya
felipemontoya approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking almost ready to me now. I left a comment about the name of the exception but once that is in, I''m good with merging.

@mariajgrimaldi
feat: add 1st batch of filters
4470dc9 
* Add PreRegisterFilter
* Add PreLoginFilter
@mariajgrimaldi mariajgrimaldi merged commit ae2ca26 into main Dec 15, 2021
@mariajgrimaldi mariajgrimaldi deleted the MJG/1st_filters_batch branch August 31, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felipemontoya felipemontoya felipemontoya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mariajgrimaldi @felipemontoya